Skip to content

Conversation

heckj
Copy link
Collaborator

@heckj heckj commented Jan 4, 2025

I may be "jumping the gun" here- but wanted to explore poking at redis locally to work out the middleware using it.

  • this adds a REDIS_HOST environment variable, and references that instead of the string "redis". I've added it to the template for dev and test, and the app.yaml, but I'm not certain about the production ENV variables and what we might want/need to do to set that to "redis" in those locations.
  • adds a redis instance in the makefile and commands to spin up and down
  • adjusts the code in configure to use the environment variable

With this in place, I was able to spin up the local environment on my machine in docker, with an additional local redis instance exposed on 6379 localhost.

@heckj heckj requested a review from finestructure January 4, 2025 21:38
@heckj heckj self-assigned this Jan 4, 2025
@cla-bot cla-bot bot added the cla-signed label Jan 4, 2025
@finestructure
Copy link
Member

Let's hold off on this, Joe. I'm preparing a first "real" use of Redis for our current documentation reference cache. This sets up the infrastructure needed (although it's quite tied to this particular use case for the moment - for example, the nested Redis actor should move to the top level once I'm done testing this setup).

@finestructure
Copy link
Member

Sorry, I just realised I misread the point of this PR, see my comment here: #3582 (comment)

I think all we need to do is weave the Environment.get("REDIS_HOST") into the Redis actor where it's using the hard-coded string right now and then we're good.

app.yml Outdated
TWITTER_API_SECRET: ${TWITTER_API_SECRET}
TWITTER_ACCESS_TOKEN_KEY: ${TWITTER_ACCESS_TOKEN_KEY}
TWITTER_ACCESS_TOKEN_SECRET: ${TWITTER_ACCESS_TOKEN_SECRET}
REDIS_HOST: ${REDIS_HOST}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We keep this list in alphabetical order to make it easier to confirm keys are present!

@heckj
Copy link
Collaborator Author

heckj commented Jan 9, 2025

First day I've been able to have any mental capacity to catch back up. Updated this PR to work over the main branch, still leaning into something to make redis available locally for dev work. Actor impl works perfectly, for me, but I wasn't entirely sure how you wanted to handle configuring it dynamically - I thought to reach for an environment variable, following the database host access pieces, but noticed that was also a dependency setup, and wasn't sure if dependencies in dependencies were even a thing, or if a straight processinfo access point inside the actor setup code would be acceptable.

@finestructure
Copy link
Member

@Dependency works inside a dependency and we should always aim to funnel side effects through them. I'll fix this up and update the Redis bit to work with these changes here.

@finestructure
Copy link
Member

Apologies for the delay Joe, I completely lost track of this one!

@finestructure finestructure merged commit f65d786 into main Jan 17, 2025
6 checks passed
@finestructure finestructure deleted the local-redis branch January 17, 2025 08:43
@daveverwer
Copy link
Member

We should also add something to the LOCAL_DEVELOPMENT_SETUP.md file that explains in what circumstances and which bits of the code you'd need a Redis server and how to use the make commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants